-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix constructor spy in unit test with Sinon 4.1.3 #7433
Fix constructor spy in unit test with Sinon 4.1.3 #7433
Conversation
When a constructor is spied using Sinon it is wrapped by a proxy function, which calls the original constructor when invoked. When "new Foo()" is executed a "Foo" object is created, "Foo" is invoked with the object as "this", and the object is returned as the result of the whole "new" expression. Before Sinon 4.1.3 the proxy called the original constructor directly using the "thisValue" of the spied call; "thisValue" was the object created by the "new" operator that called the proxy. The proxy assigned "thisValue" to "returnValue", so it was also the value returned by the proxy and, in turn, the value returned by the whole "new" expression. Since Sinon 4.1.3 (see pull request 1626) the proxy calls the original constructor using "new" instead of directly. The "thisValue" created by the outermost "new" (the one that called the proxy) is no longer used by the original constructor; the internal "new" creates a new object, which is the one passed to the original constructor and returned by the internal "new" expression. This object is also the value returned by the proxy ("returnValue") and, in turn, the value returned by the whole outermost "new" expression. Thus, now "returnValue" should be used instead of "thisValue" to get the object created by the spied constructor. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #7433 +/- ##
============================================
+ Coverage 50.42% 50.94% +0.51%
Complexity 24717 24717
============================================
Files 1522 1586 +64
Lines 86432 94163 +7731
Branches 0 1365 +1365
============================================
+ Hits 43587 47971 +4384
- Misses 42845 46192 +3347
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seriously, are you still reading this? Well, thanks :-) It is great that someone else besides me read it; it seems that I did not waste my time after all :-P
😜
Anyways thanks a lot 👍
I'm confused. Is the new behavior better or worse? |
I would say just different. According to your pull request calling |
stable12 backport: #7451 |
TL;DR;
When spying on a constructor using Sinon the objects created by the constructor are got using
spy.getCall(XXX).returnValue
.All the long and boring details
As explained in the MDN Web Docs,
new Foo(...)
is executed in three steps:Foo.prototype
is created.Foo
is called withthis
bound to that new object.When a constructor (or any other function) is spied using Sinon the original constructor is replaced by a proxy. When the proxy is called it will record the arguments it was called with, call the original constructor, and finally record the value returned or the exceptions thrown by the original constructor.
Before Sinon 4.1.3 when the proxy was called using the
new
operator the proxy called the original constructor directly, passing it thethisValue
, which was the object created in step 1 above. If the original constructor function did not return an object (and note that this is the most common case, as the value returned by a constructor is usuallyundefined
; as explained abovenew
is the one that returns the created object when the constructor returns no value) thenreturnValue
was set tothisValue
; this is the value returned by the proxy tonew
, and thus is the value returned by the wholenew
expression as explained in the step 3. Therefore, before Sinon 4.1.3,spy.getCall(XXX).thisValue
andspy.getCall(XXX).returnValue
had (if the constructor returned no value, like in the failing test) the same value.This has changed in Sinon 4.1.3, as now when the proxy is called using the
new
operator the proxy calls the original constructor usingnew
too. Whennew
is called inside the proxy the three steps outlined above happen again: a new object is created, the original constructor is called withthis
bound to that object, and the object is returned as the result of the wholenew (bind.apply(this.func || func, [thisValue].concat(args)))()
expression; asreturnValue
is an object it is not replaced bythisValue
and thusreturnValue
is the value returned by the proxy to thenew
operator that called it.Hey, wait a minute, when the original constructor is called inside the proxy it is bound to
thisValue
, so whennew
calls the original constructor in the step 2 it will usethisValue
instead of the newly created object asthis
, right? Nope, as explained again in the MDN Web Docs when a function is boundthis
is ignored if the function is called bynew
.Therefore, in Sinon 4.1.3,
thisValue
contains the object created bynew proxy
, butreturnValue
contains the real object created bynew originalConstructor
inside the proxy and also returned by the outermostnew
expression. That is,returnValue
is the one you want to use.Seriously, are you still reading this? Well, thanks :-) It is great that someone else besides me read it; it seems that I did not waste my time after all :-P